This could be avoid by introducing a dry-run ante handler that, othen than simulating sig verification, would use an infinite gas meter as well.

Why do we need this at all? If the user doesn't want to simulate the transaction I think we should force them to manually specify gas.

Makes sense, I will change it accordingly.
I was thinking to drop the --gas-auto flag in favor of having --gas value set to 0 by default, which would enable the auto gas feature.

I don't think a limit makes sense here either, can we use an InfiniteGasMeter on simulation?

Addressed, please review

Can this be a CLI flag (1.2 is probably a fine default value)?

How about --gas-adjustment?

Sounds good

Hmm I'm not sure this is the optimal way to structure this function set, now there seems to be some duplicate code in simulateTx.

Addressing this now, expect a commit soon

Rename to TestParseQueryResponse

typo here initialize

Don't we need to add the --gas-adjustement flag as well ?

Done

Cool ! could we move this to another pkg and export it ?

use your writeErr function

use writeErr

same

Lets add some comment here about what the intended result of this is under each mode.

I agree with the code change, however this code is hard to grok, especially now that we've removed the reference to check/simulate.

Incorporated, thanks

Why is this called applyTxMode if it only does stuff for simulate? It seems confusing imo to generalize this when there is only one case. We're also not really applying the tx mode imo, its just initializing the context.

Perhaps rename this to initializeTxMode?

How about initializeContext()?

Perhaps indicate that this is a multiplicative factor being multiplied against the estimate (vs additive).

ACK'd

Lets test success with sufficient, manually specified gas as well?

ACKd

We can eliminate some of the code duplication here by just making the line "gas":"%v" a fmt string arg, and just set that at the top, and pass that in. (so if gas > 0, that argument would be set to "gas":"\n", otherwise it would be set to "".

ACKd

Should we write up into an issue to make this configurable per tx type in a config somewhere? The reason for considering it would be that a bank send tx probably is going to be most common tx, and also shouldn't have an increased cost due to other state changes, so it shouldn't have the same adjustment as staking txs.

Makes sense. Please fork it into a new issue 👍

Yeah, this is definitely something to consider because the user pays for the allocated gas, not the consumed gas.

#2140

Why is this here? Using an infinite gas meter here seems odd to me.

When running in simulation mode, we want no gas capping whatsoever - CC'ing @cwgoes

We don't need to cap gas in a simulation, we just need to measure it - is there a reason you think we would need to cap it @ValarDragon?

I don't like switching on gas equals 0. This feels like it could cause more bugs / increase the attack surface. I think we should find another way of communicating that were in simulation mode.

Could it not be set in the context somewhere upstream?

What if we use a default gas limit (some sort of sane default, e.g. 200000 - which is the current default value) and switch back to use a capped GasMeter?

Re:

simulations shouldn't be verifying signatures

That too is tricky. Simulations go through the query interface, which requires messages to be signed.

That still doesn't really alleviate my concern. (That does limit the situation though)

I do think switching on 0 gas is less safe, as we now have to make sure that a tx on the network with 0 gas is stopped before getting to this line. Its prefferrable to just have a separate method to flag simulations, and have the normal gas logic prevent a 0 gas tx imo.

Do you reckon the following could mitigate the risk?

diff --git a/client/utils/utils.go b/client/utils/utils.go
index fb5d6198..49258b86 100644
--- a/client/utils/utils.go
+++ b/client/utils/utils.go
@@ -1,6 +1,7 @@
 package utils
 
 import (
+       "errors"
        "fmt"
        "os"
 
@@ -108,6 +109,10 @@ func CalculateGas(queryFunc func(string, common.HexBytes) ([]byte, error), cdc *
        if err != nil {
                return
        }
+       if estimate <= 0 {
+               err = errors.New("simulation returned gas=0")
+               return
+       }
        adjusted = adjustGasEstimate(estimate, adjustment)
        fmt.Fprintf(os.Stderr, "gas: [estimated = %v] [adjusted = %v]\n", estimate, adjusted)
        return

I think I'm not conveying my concern well. This doesn't really alleviate it.

Right now the current code is saying "If tx.fee.Gas = 0, I get infinite gas". I think this is a bad design for it, even if we add checks in other places. As now were opening more holes, where are a complex series of conditionals could lead to this gas=0, get infinite gas, or a change may happen later that forgets that this could be the case. Instead we should signal directly that we want simulation mode, so this is no longer a fear. (As a tx itself cannot signal simulation mode, so theres no safety risk there)

I agree that this isn't a simple change, but I think its necessary for this to go through. I also think requiring that simulation not take in the private key / a signature is necessary, even if it requires another AnteHandler constructor. You don't want to increase the amount of things a private key has to sign / increase the amount of exposure a private key has if you don't have to.

Why can't 0 be used for the fee here?

0 gas triggers the simulation

Lets move this refactor into a seperate PR.

requested by @fedekunze - I'd keep it if you don't mind as IMHO it does more good than bad

yet I'll refrain in future to tackle technical debt while working on a specific issue 👍

EnrichTxContextWithGas feels like an odd name for whats happening here. I think it should be renamed to use the word Calculate. Also this should be refactored to not depend on the password imo. (We should not increase the surface area that gets password access) We can charge gas purely on signature type. (As simulation shouldn't spend the signature verification time anyway)

How about the following?

// BuildAndSignTxWithZeroGas builds transactions with GasWanted set to 0.
func BuildAndSignTxWithZeroGas(txCtx authctx.TxContext, name, passphrase string, msgs []sdk.Msg) ([]byte, error) {
	return txCtx.WithGas(0).BuildAndSign(name, passphrase, msgs)
}

// EnrichTxContextWithGas simulates the execution of a transaction to
// then populate the relevant TxContext.Gas field with the estimate
// obtained by the query.
func EnrichTxContextWithGas(txCtx authctx.TxContext, cliCtx context.CLIContext, txBytes []byte) (authctx.TxContext, error) {
	// run a simulation (via /app/simulate query) to
	// estimate gas and update TxContext accordingly
	rawRes, err := cliCtx.Query("/app/simulate", txBytes)
	if err != nil {
		return txCtx, err
	}
	estimate, err := parseQueryResponse(cliCtx.Codec, rawRes)
	if err != nil {
		return txCtx, err
	}
	adjusted := adjustGasEstimate(estimate, cliCtx.GasAdjustment)
	fmt.Fprintf(os.Stderr, "gas: [estimated = %v] [adjusted = %v]\n", estimate, adjusted)
	return txCtx.WithGas(adjusted), nil
}

I disagree with needing a signature here. We don't want simulations to depend on the private key. (Extra computation time, inconvenience, etc. Especially if your signing from a ledger)

(nit) Writing this with an if mode == runTxModeSimulate is a bit more obvious (that we're doing something special for simulation).

Incorporated, thanks.

(nit) Writing this with if mode == runTxModeDelvier is a bit more obvious (that we're doing something special for delivertx)

Incorporated, thanks

I thought this was 1.2 (below)?

Might be slightly clearer to put this conditional outside of the function, and only call the function if cliCtx.Gas == 0

As long as linter doesn't complain yes, that was my intent

Yeah, this is definitely something to consider because the user pays for the allocated gas, not the consumed gas.

Can we deduplicate this function (put it in a common file)?

Dedup'd, though left it there for now if that is OK for you